-
Notifications
You must be signed in to change notification settings - Fork 8.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[ES|QL] Disable the filter actions for multivalue fields #193415
[ES|QL] Disable the filter actions for multivalue fields #193415
Conversation
Pinging @elastic/kibana-esql (Team:ESQL) |
I think it would be still helpful to pick at least the first value from the array and use that in |
@jughosta as the full text search comes soon I want to do this correctly. Only the first value doesnt return what they want so I dont think is a good decision. The users expect to find the documents that have the exact match of the multivalue and this will come with full text search and the QSTR function. When we will have this, we will have the same experience with the dataview mode. The |
Have a question, quickly testing it. First we should aim to to not show the filter icon if filtering is no possible (and it was already discussed in the issue, it shouldn't be a large effort checking for an array in the value, right @davismcphee @jughosta, but of course I might miss a 🐰 🕳️ ). So, if we can't add this to this PR (which aims to move fast 🏃 ), we are creating to a state where we currently don't give any user feedback what's happening, or how it could be fixed. So while I prefer toasts for breakfast, how about a toast here when it is clicked (however i struggle to find words, I must admit)? Or can we think of another user feedback preventing a non-functional button? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While we can't fully hide the filter icons due to the way EUI data grid works, rather than silently ignoring the user's action, how about we disable the buttons and inform the user it's unsupported?
I opened a PR against this branch if we all agree with this approach: stratoula#35.
…es-esql [Discover] [Unified Data Table] Disable filtering for multivalue fields in ES|QL mode
Awesome @davismcphee 🙇♀️ |
/ci |
@davismcphee so happy nobody listend to me! Toast invasion prevented! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure this makes sense!
I assume it's more work to also enable this for the DocViewer (with a quick look at the code), however I think we should be able to bring this over the finishing line before 8.16, wdyt @davismcphee ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with both of you @stratoula and @kertal 🙂 Let's merge this one as is and I'll open a followup to do the same for the doc viewer 👍
💛 Build succeeded, but was flaky
Failed CI StepsTest Failures
Metrics [docs]Async chunks
Page load bundle
History
To update your PR or re-run it, just comment with: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, code review only.
) ## Summary Part of elastic#193015 It not allows the creation of where clause filters in case of multi value fields as this is not supported yet in ES|QL. Check my comment here elastic#193015 (comment) It might be possible with full text search but I need to talk to the team first. For now we disable it as it creates a wrong filter. ### Checklist - [ ] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios --------- Co-authored-by: kibanamachine <[email protected]> Co-authored-by: Davis McPhee <[email protected]> (cherry picked from commit 2492981)
💚 All backports created successfully
Note: Successful backport PRs will be merged automatically after passing CI. Questions ?Please refer to the Backport tool documentation |
## Summary As the title says. Two competing changes made it in to main (#193415 & #193791), causing a type issue: https://buildkite.com/elastic/kibana-on-merge/builds/50786#019228ae-4487-45c6-867a-ba0590b1266d cc: @davismcphee @stratoula
…) (#193962) # Backport This will backport the following commits from `main` to `8.x`: - [[ES|QL] Disable the filter actions for multivalue fields (#193415)](#193415) <!--- Backport version: 9.4.3 --> ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport) <!--BACKPORT [{"author":{"name":"Stratoula Kalafateli","email":"[email protected]"},"sourceCommit":{"committedDate":"2024-09-25T10:14:34Z","message":"[ES|QL] Disable the filter actions for multivalue fields (#193415)\n\n## Summary\r\n\r\nPart of https://github.com/elastic/kibana/issues/193015\r\n\r\nIt not allows the creation of where clause filters in case of multi\r\nvalue fields as this is not supported yet in ES|QL. Check my comment\r\nhere\r\nhttps://github.com//issues/193015#issuecomment-2360704651\r\n\r\nIt might be possible with full text search but I need to talk to the\r\nteam first. For now we disable it as it creates a wrong filter.\r\n\r\n### Checklist\r\n\r\n- [ ] [Unit or functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere updated or added to match the most common scenarios\r\n\r\n---------\r\n\r\nCo-authored-by: kibanamachine <[email protected]>\r\nCo-authored-by: Davis McPhee <[email protected]>","sha":"249298166fe1499b4aea501056a2291711427c5b","branchLabelMapping":{"^v9.0.0$":"main","^v8.16.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:skip","v9.0.0","backport:prev-minor","Feature:ES|QL","Team:ESQL","v8.16.0"],"title":"[ES|QL] Disable the filter actions for multivalue fields","number":193415,"url":"https://github.com/elastic/kibana/pull/193415","mergeCommit":{"message":"[ES|QL] Disable the filter actions for multivalue fields (#193415)\n\n## Summary\r\n\r\nPart of https://github.com/elastic/kibana/issues/193015\r\n\r\nIt not allows the creation of where clause filters in case of multi\r\nvalue fields as this is not supported yet in ES|QL. Check my comment\r\nhere\r\nhttps://github.com//issues/193015#issuecomment-2360704651\r\n\r\nIt might be possible with full text search but I need to talk to the\r\nteam first. For now we disable it as it creates a wrong filter.\r\n\r\n### Checklist\r\n\r\n- [ ] [Unit or functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere updated or added to match the most common scenarios\r\n\r\n---------\r\n\r\nCo-authored-by: kibanamachine <[email protected]>\r\nCo-authored-by: Davis McPhee <[email protected]>","sha":"249298166fe1499b4aea501056a2291711427c5b"}},"sourceBranch":"main","suggestedTargetBranches":["8.x"],"targetPullRequestStates":[{"branch":"main","label":"v9.0.0","branchLabelMappingKey":"^v9.0.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/193415","number":193415,"mergeCommit":{"message":"[ES|QL] Disable the filter actions for multivalue fields (#193415)\n\n## Summary\r\n\r\nPart of https://github.com/elastic/kibana/issues/193015\r\n\r\nIt not allows the creation of where clause filters in case of multi\r\nvalue fields as this is not supported yet in ES|QL. Check my comment\r\nhere\r\nhttps://github.com//issues/193015#issuecomment-2360704651\r\n\r\nIt might be possible with full text search but I need to talk to the\r\nteam first. For now we disable it as it creates a wrong filter.\r\n\r\n### Checklist\r\n\r\n- [ ] [Unit or functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere updated or added to match the most common scenarios\r\n\r\n---------\r\n\r\nCo-authored-by: kibanamachine <[email protected]>\r\nCo-authored-by: Davis McPhee <[email protected]>","sha":"249298166fe1499b4aea501056a2291711427c5b"}},{"branch":"8.x","label":"v8.16.0","branchLabelMappingKey":"^v8.16.0$","isSourceBranch":false,"state":"NOT_CREATED"}]}] BACKPORT--> --------- Co-authored-by: Stratoula Kalafateli <[email protected]>
…Viewer (#194232) ## Summary This PR disables ES|QL multivalue filtering in Unified Doc Viewer, similar to what we did for Unified Data Table in #193415: ![image](https://github.com/user-attachments/assets/f86c3ba9-05e9-4245-97f6-a8f9413950e9) Part of #193015. ### Checklist - [x] Any text added follows [EUI's writing guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses sentence case text and includes [i18n support](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md) - [ ] [Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html) was added for features that require explanation or tutorials - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios - [ ] [Flaky Test Runner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was used on any tests changed - [x] Any UI touched in this PR is usable by keyboard only (learn more about [keyboard accessibility](https://webaim.org/techniques/keyboard/)) - [ ] Any UI touched in this PR does not create any new axe failures (run axe in browser: [FF](https://addons.mozilla.org/en-US/firefox/addon/axe-devtools/), [Chrome](https://chrome.google.com/webstore/detail/axe-web-accessibility-tes/lhdoppojpmngadmnindnejefpokejbdd?hl=en-US)) - [ ] If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the [docker list](https://github.com/elastic/kibana/blob/main/src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker) - [ ] This renders correctly on smaller devices using a responsive layout. (You can test this [in your browser](https://www.browserstack.com/guide/responsive-testing-on-local-server)) - [ ] This was checked for [cross-browser compatibility](https://www.elastic.co/support/matrix#matrix_browsers) ### For maintainers - [ ] This was checked for breaking API changes and was [labeled appropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)
…Viewer (elastic#194232) ## Summary This PR disables ES|QL multivalue filtering in Unified Doc Viewer, similar to what we did for Unified Data Table in elastic#193415: ![image](https://github.com/user-attachments/assets/f86c3ba9-05e9-4245-97f6-a8f9413950e9) Part of elastic#193015. ### Checklist - [x] Any text added follows [EUI's writing guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses sentence case text and includes [i18n support](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md) - [ ] [Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html) was added for features that require explanation or tutorials - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios - [ ] [Flaky Test Runner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was used on any tests changed - [x] Any UI touched in this PR is usable by keyboard only (learn more about [keyboard accessibility](https://webaim.org/techniques/keyboard/)) - [ ] Any UI touched in this PR does not create any new axe failures (run axe in browser: [FF](https://addons.mozilla.org/en-US/firefox/addon/axe-devtools/), [Chrome](https://chrome.google.com/webstore/detail/axe-web-accessibility-tes/lhdoppojpmngadmnindnejefpokejbdd?hl=en-US)) - [ ] If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the [docker list](https://github.com/elastic/kibana/blob/main/src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker) - [ ] This renders correctly on smaller devices using a responsive layout. (You can test this [in your browser](https://www.browserstack.com/guide/responsive-testing-on-local-server)) - [ ] This was checked for [cross-browser compatibility](https://www.elastic.co/support/matrix#matrix_browsers) ### For maintainers - [ ] This was checked for breaking API changes and was [labeled appropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process) (cherry picked from commit 6f01d6a)
…d Doc Viewer (#194232) (#194325) # Backport This will backport the following commits from `main` to `8.x`: - [[Discover] [ES|QL] Disable ES|QL multivalue filtering in Unified Doc Viewer (#194232)](#194232) <!--- Backport version: 9.4.3 --> ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport) <!--BACKPORT [{"author":{"name":"Davis McPhee","email":"[email protected]"},"sourceCommit":{"committedDate":"2024-09-27T16:28:48Z","message":"[Discover] [ES|QL] Disable ES|QL multivalue filtering in Unified Doc Viewer (#194232)\n\n## Summary\r\n\r\nThis PR disables ES|QL multivalue filtering in Unified Doc Viewer,\r\nsimilar to what we did for Unified Data Table in #193415:\r\n\r\n![image](https://github.com/user-attachments/assets/f86c3ba9-05e9-4245-97f6-a8f9413950e9)\r\n\r\nPart of https://github.com/elastic/kibana/issues/193015.\r\n\r\n### Checklist\r\n\r\n- [x] Any text added follows [EUI's writing\r\nguidelines](https://elastic.github.io/eui/#/guidelines/writing), uses\r\nsentence case text and includes [i18n\r\nsupport](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md)\r\n- [ ]\r\n[Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html)\r\nwas added for features that require explanation or tutorials\r\n- [x] [Unit or functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere updated or added to match the most common scenarios\r\n- [ ] [Flaky Test\r\nRunner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was\r\nused on any tests changed\r\n- [x] Any UI touched in this PR is usable by keyboard only (learn more\r\nabout [keyboard accessibility](https://webaim.org/techniques/keyboard/))\r\n- [ ] Any UI touched in this PR does not create any new axe failures\r\n(run axe in browser:\r\n[FF](https://addons.mozilla.org/en-US/firefox/addon/axe-devtools/),\r\n[Chrome](https://chrome.google.com/webstore/detail/axe-web-accessibility-tes/lhdoppojpmngadmnindnejefpokejbdd?hl=en-US))\r\n- [ ] If a plugin configuration key changed, check if it needs to be\r\nallowlisted in the cloud and added to the [docker\r\nlist](https://github.com/elastic/kibana/blob/main/src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker)\r\n- [ ] This renders correctly on smaller devices using a responsive\r\nlayout. (You can test this [in your\r\nbrowser](https://www.browserstack.com/guide/responsive-testing-on-local-server))\r\n- [ ] This was checked for [cross-browser\r\ncompatibility](https://www.elastic.co/support/matrix#matrix_browsers)\r\n\r\n### For maintainers\r\n\r\n- [ ] This was checked for breaking API changes and was [labeled\r\nappropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)","sha":"6f01d6a2605e69737ddff0993fc07d8ab8deb43e","branchLabelMapping":{"^v9.0.0$":"main","^v8.16.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["Feature:Discover","release_note:skip","v9.0.0","Team:DataDiscovery","backport:prev-minor","Feature:UnifiedDocViewer","Feature:ES|QL","Team:ESQL"],"title":"[Discover] [ES|QL] Disable ES|QL multivalue filtering in Unified Doc Viewer","number":194232,"url":"https://github.com/elastic/kibana/pull/194232","mergeCommit":{"message":"[Discover] [ES|QL] Disable ES|QL multivalue filtering in Unified Doc Viewer (#194232)\n\n## Summary\r\n\r\nThis PR disables ES|QL multivalue filtering in Unified Doc Viewer,\r\nsimilar to what we did for Unified Data Table in #193415:\r\n\r\n![image](https://github.com/user-attachments/assets/f86c3ba9-05e9-4245-97f6-a8f9413950e9)\r\n\r\nPart of https://github.com/elastic/kibana/issues/193015.\r\n\r\n### Checklist\r\n\r\n- [x] Any text added follows [EUI's writing\r\nguidelines](https://elastic.github.io/eui/#/guidelines/writing), uses\r\nsentence case text and includes [i18n\r\nsupport](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md)\r\n- [ ]\r\n[Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html)\r\nwas added for features that require explanation or tutorials\r\n- [x] [Unit or functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere updated or added to match the most common scenarios\r\n- [ ] [Flaky Test\r\nRunner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was\r\nused on any tests changed\r\n- [x] Any UI touched in this PR is usable by keyboard only (learn more\r\nabout [keyboard accessibility](https://webaim.org/techniques/keyboard/))\r\n- [ ] Any UI touched in this PR does not create any new axe failures\r\n(run axe in browser:\r\n[FF](https://addons.mozilla.org/en-US/firefox/addon/axe-devtools/),\r\n[Chrome](https://chrome.google.com/webstore/detail/axe-web-accessibility-tes/lhdoppojpmngadmnindnejefpokejbdd?hl=en-US))\r\n- [ ] If a plugin configuration key changed, check if it needs to be\r\nallowlisted in the cloud and added to the [docker\r\nlist](https://github.com/elastic/kibana/blob/main/src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker)\r\n- [ ] This renders correctly on smaller devices using a responsive\r\nlayout. (You can test this [in your\r\nbrowser](https://www.browserstack.com/guide/responsive-testing-on-local-server))\r\n- [ ] This was checked for [cross-browser\r\ncompatibility](https://www.elastic.co/support/matrix#matrix_browsers)\r\n\r\n### For maintainers\r\n\r\n- [ ] This was checked for breaking API changes and was [labeled\r\nappropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)","sha":"6f01d6a2605e69737ddff0993fc07d8ab8deb43e"}},"sourceBranch":"main","suggestedTargetBranches":[],"targetPullRequestStates":[{"branch":"main","label":"v9.0.0","branchLabelMappingKey":"^v9.0.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/194232","number":194232,"mergeCommit":{"message":"[Discover] [ES|QL] Disable ES|QL multivalue filtering in Unified Doc Viewer (#194232)\n\n## Summary\r\n\r\nThis PR disables ES|QL multivalue filtering in Unified Doc Viewer,\r\nsimilar to what we did for Unified Data Table in #193415:\r\n\r\n![image](https://github.com/user-attachments/assets/f86c3ba9-05e9-4245-97f6-a8f9413950e9)\r\n\r\nPart of https://github.com/elastic/kibana/issues/193015.\r\n\r\n### Checklist\r\n\r\n- [x] Any text added follows [EUI's writing\r\nguidelines](https://elastic.github.io/eui/#/guidelines/writing), uses\r\nsentence case text and includes [i18n\r\nsupport](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md)\r\n- [ ]\r\n[Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html)\r\nwas added for features that require explanation or tutorials\r\n- [x] [Unit or functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere updated or added to match the most common scenarios\r\n- [ ] [Flaky Test\r\nRunner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was\r\nused on any tests changed\r\n- [x] Any UI touched in this PR is usable by keyboard only (learn more\r\nabout [keyboard accessibility](https://webaim.org/techniques/keyboard/))\r\n- [ ] Any UI touched in this PR does not create any new axe failures\r\n(run axe in browser:\r\n[FF](https://addons.mozilla.org/en-US/firefox/addon/axe-devtools/),\r\n[Chrome](https://chrome.google.com/webstore/detail/axe-web-accessibility-tes/lhdoppojpmngadmnindnejefpokejbdd?hl=en-US))\r\n- [ ] If a plugin configuration key changed, check if it needs to be\r\nallowlisted in the cloud and added to the [docker\r\nlist](https://github.com/elastic/kibana/blob/main/src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker)\r\n- [ ] This renders correctly on smaller devices using a responsive\r\nlayout. (You can test this [in your\r\nbrowser](https://www.browserstack.com/guide/responsive-testing-on-local-server))\r\n- [ ] This was checked for [cross-browser\r\ncompatibility](https://www.elastic.co/support/matrix#matrix_browsers)\r\n\r\n### For maintainers\r\n\r\n- [ ] This was checked for breaking API changes and was [labeled\r\nappropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)","sha":"6f01d6a2605e69737ddff0993fc07d8ab8deb43e"}}]}] BACKPORT--> Co-authored-by: Davis McPhee <[email protected]>
Summary
Part of #193015
It not allows the creation of where clause filters in case of multi value fields as this is not supported yet in ES|QL. Check my comment here #193015 (comment)
It might be possible with full text search but I need to talk to the team first. For now we disable it as it creates a wrong filter.
Checklist